-
Notifications
You must be signed in to change notification settings - Fork 5.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Document patch strategy markers in api-conventions #4218
[WIP] Document patch strategy markers in api-conventions #4218
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mariantalla The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mariantalla The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
OK looks good to me, @liggitt is this relevant here? |
It mixes up a couple of different concepts though...needs some more work.
@@ -591,6 +591,33 @@ saved. For more details on how to use Merge Patch, see the RFC. | |||
detailed explanation of how it works and why it needed to be introduced, see | |||
[here](/contributors/devel/sig-api-machinery/strategic-merge-patch.md). | |||
|
|||
#### Declaring Patch Strategy | |||
In the context of [strategic merge patch](/contributors/devel/sig-api-machinery/strategic-merge-patch.md), a set of go markers can be used by API authors to declare the patch strategy of collections of fields (lists, maps and structs). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this sentence is correct, listType
etc don't specifically make sense in the context of strategic merge.
@@ -591,6 +591,33 @@ saved. For more details on how to use Merge Patch, see the RFC. | |||
detailed explanation of how it works and why it needed to be introduced, see | |||
[here](/contributors/devel/sig-api-machinery/strategic-merge-patch.md). | |||
|
|||
#### Declaring Patch Strategy | |||
In the context of [strategic merge patch](/contributors/devel/sig-api-machinery/strategic-merge-patch.md), a set of go markers can be used by API authors to declare the patch strategy of collections of fields (lists, maps and structs). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these directives are only relevant for server-side apply, not strategic merge patch. @apelisse, can you give guidance on which apply to which?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I made pretty much the same statement above.
|
||
| Marker | Definition | | ||
|--------|------------| | ||
| `//+patchStrategy=merge/replace` | Defines that the merge strategy will be to: merge elements of the two lists into one (if set to merge), or use the list provided in the patch literally rather than merging (if set to replace). Default is `replace`. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be more readable to give each directive/value pair its own line
|
||
A newer version of an API may decide to redefine the patch strategy or topology for a list, map, or struct. This section lists some scenarios and expected behaviours. | ||
|
||
* A `listType` can only be updated from `atomic` to `set` if the list elements are scalars. Similarly, `listType` updates from `atomic` to `map` will only succeed if the list items are key-value pairs, and a `listMapKeys` marker is added to define the list of keys that uniquely identify a list entry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clarify can vs should... you can do some things that you should not do
* A `listType` can only be updated from `atomic` to `set` if the list elements are scalars. Similarly, `listType` updates from `atomic` to `map` will only succeed if the list items are key-value pairs, and a `listMapKeys` marker is added to define the list of keys that uniquely identify a list entry. | ||
* When updating `listType` from `set` to `atomic`, the next actor (e.g. `kubectl`) to apply the list will entirely replace it. The operation will not give out conflicts, even if the actor has't previously issued requests against this field. | ||
* When using a Kubernetes API version that does not support setting list, map and struct topologies (1.15 and before): lists, maps and structs are considered atomic. When upgrading to an API version that supports declaring topology for these fields (starting at 1.16): | ||
* if `listType` is set to `set` or `map` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this line mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I think I've stopped mid-sentence there, sorry :/ I've removed this whole section on updates while we figure them out.
* When updating `listType` from `set` to `atomic`, the next actor (e.g. `kubectl`) to apply the list will entirely replace it. The operation will not give out conflicts, even if the actor has't previously issued requests against this field. | ||
* When using a Kubernetes API version that does not support setting list, map and struct topologies (1.15 and before): lists, maps and structs are considered atomic. When upgrading to an API version that supports declaring topology for these fields (starting at 1.16): | ||
* if `listType` is set to `set` or `map` | ||
* if `listType` is set to `atomic`, `apply` operations using server-side apply will result in conflicts, unless they leave the field values unchanged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apelisse is this true? that's really confusing if the fields were considered atomic previously
* When using a Kubernetes API version that does not support setting list, map and struct topologies (1.15 and before): lists, maps and structs are considered atomic. When upgrading to an API version that supports declaring topology for these fields (starting at 1.16): | ||
* if `listType` is set to `set` or `map` | ||
* if `listType` is set to `atomic`, `apply` operations using server-side apply will result in conflicts, unless they leave the field values unchanged. | ||
* Updates from specifying topologies to _not_ specifying them, will be equivalent to the topologies being set to `atomic`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apelisse - being set to atomic
(visible in the API) or treated as atomic
(behavior in the server)?
* if `listType` is set to `set` or `map` | ||
* if `listType` is set to `atomic`, `apply` operations using server-side apply will result in conflicts, unless they leave the field values unchanged. | ||
* Updates from specifying topologies to _not_ specifying them, will be equivalent to the topologies being set to `atomic`. | ||
* Updates between scalar and nested types are allowed, but are risky. If such a path must be followed, it's best to test outside a production system, and configure to `atomic` as a middle step. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like it falls under the same category of things you should not do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
configure to
atomic
as a middle step
what does this accomplish?
* Updates from specifying topologies to _not_ specifying them, will be equivalent to the topologies being set to `atomic`. | ||
* Updates between scalar and nested types are allowed, but are risky. If such a path must be followed, it's best to test outside a production system, and configure to `atomic` as a middle step. | ||
* From `set` to `map`: while new objects can be created without problems following the updated topology, existing objects can no longer be changed. | ||
* From `map` to `set`: there's high risk of losing or corrupting data of existing objects. Just reconfiguring `listType` without reapplying objects will empty the contents of existing objects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just reconfiguring
listType
without reapplying objects will empty the contents of existing objects.
@apelisse - Is this accurate? I'm not aware of any process that does this. Does this actually mean "empty contents" or does it mean "deduplicate items to form a set"?
* Updates between scalar and nested types are allowed, but are risky. If such a path must be followed, it's best to test outside a production system, and configure to `atomic` as a middle step. | ||
* From `set` to `map`: while new objects can be created without problems following the updated topology, existing objects can no longer be changed. | ||
* From `map` to `set`: there's high risk of losing or corrupting data of existing objects. Just reconfiguring `listType` without reapplying objects will empty the contents of existing objects. | ||
* Updating `listMapKeys` for a `listType=map` will succeed as long as the spec of the existing objects is valid by both key validations; the original and the update. For example, with an original configuration of `listMapKeys=port,protocol` and an updated configuration of `listMapKeys=name,version`, list entries of an existing object must have unique, not omitted combinations of both `port,protocol` and `name,version`; otherwise the object can't be updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating
listMapKeys
for alistType=map
will succeed as long as the spec of the existing objects is valid by both key validations;
This makes it sound like the server will check existing data for you and prevent you from making unsafe changes to the schema which is not the case. There is no validation of existing objects performed when updating a custom resource schema.
I see that you've added the part about compatibility, which is great. But I'd much rather see the test before to be convinced about the validity. I'll look for the tests first. Can we maybe split this PR in 2? 1 part that explains how the markers work, and later another PR that explains the compatibility? |
for readability
Thanks for the feedback! I've removed the compatibility part while we sort it out, and rephrased points that were unclear or wrong. Let me know how that looks! |
| `//+patchStrategy=merge` | Defines that the merge strategy will be to merge elements of the two lists into one. | | ||
| `//+patchStrategy=replace` (default)| Defines that the merge strategy will be touse the list provided in the patch literally rather than merging. | | ||
| `// +patchMergeKey=<key-name>` | Applicable in the context where lists are used as maps/dictionaries, i.e. where each list entry is a key-value pair. `patchMergeKey` defines which field should be seen as the key. Combined with `patchStrategy`, this describes what changes should take place on specific elements of a list as part of a patch operation. | | ||
| `//+listType=atomic/set/map` | Applicable to objects that are of type list. A single actor can replace the entire list, but cannot change individual elements of it. Unless set to `atomic`, different actors can update individual elements in the list. In that case, the listType can either be `set` (contains scalar elements only) or `map` (contains map-like elements). | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applicable to objects that are of type list. A single actor can replace the entire list, but cannot change individual elements of it. Unless set to
atomic
, different actors can update individual elements in the list.
I find this a little confusing, since you start by saying that a signle actor can replace the entire list.
What about this:
Applicable to objects that are of type list. Defines if list are owned as a whole by a single actor of if each individual item is owned by a different actor.
atomic
means that the list is owned/changed entirely by a single actor.set
means that each item in the list can be owned by a different actor, the items of the list must be atomic/scalar.map
means that the list is an associative list, each actor can own a separate item in the list, the keys have to be scalars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, I meant to make that line talk about atomic
only, but didn't do it all the way. Fixed now, I think.
| `//+patchStrategy=replace` (default)| Defines that the merge strategy will be touse the list provided in the patch literally rather than merging. | | ||
| `// +patchMergeKey=<key-name>` | Applicable in the context where lists are used as maps/dictionaries, i.e. where each list entry is a key-value pair. `patchMergeKey` defines which field should be seen as the key. Combined with `patchStrategy`, this describes what changes should take place on specific elements of a list as part of a patch operation. | | ||
| `//+listType=atomic/set/map` | Applicable to objects that are of type list. A single actor can replace the entire list, but cannot change individual elements of it. Unless set to `atomic`, different actors can update individual elements in the list. In that case, the listType can either be `set` (contains scalar elements only) or `map` (contains map-like elements). | | ||
| `//+listType=set` | Applicable to objects that are of type list, and the elements are scalar. Different actors can update individual elements in the list. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elements can also be 'atomic' things. And also, elements have to be unique.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the uniqueness, not sure about the atomic things though - doesn't elements are scalar
guarantee that? Do you mean e.g. having a set
with atomic maps? Or did I misunderstand your comment? 🤔
| `//+listType=atomic/set/map` | Applicable to objects that are of type list. A single actor can replace the entire list, but cannot change individual elements of it. Unless set to `atomic`, different actors can update individual elements in the list. In that case, the listType can either be `set` (contains scalar elements only) or `map` (contains map-like elements). | | ||
| `//+listType=set` | Applicable to objects that are of type list, and the elements are scalar. Different actors can update individual elements in the list. | | ||
| `//+listType=map` | Applicable to objects that are of type list, and elements are map-like, i.e. lists that are used as dictionaries. Different actors can update individual elements in the list. | | ||
| `//+listMapKeys=<list of keys>` | Applicable in the context where `listType=map`. `listMapKeys` a list of strings, that defines which combination of keys should be used as the identifier of each element. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
combinations of keys have to be unique, keys have to be scalar. I don't know if they strictly have to be strings, but they should definitely be scalars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the "combinations must be unique" point. The keys' "names" have to be strings* but the values can indeed be anything that's scalar - I've clarified that. Thank you!
*e.g. listMapKeys: [1,2]
is invalid
| `//+patchStrategy=replace` (default)| Defines that the merge strategy will be touse the list provided in the patch literally rather than merging. | | ||
| `// +patchMergeKey=<key-name>` | Applicable in the context where lists are used as maps/dictionaries, i.e. where each list entry is a key-value pair. `patchMergeKey` defines which field should be seen as the key. Combined with `patchStrategy`, this describes what changes should take place on specific elements of a list as part of a patch operation. | | ||
| `//+listType=atomic` | Applicable to objects that are of type list. A single actor owns and can replace the entire list, but cannot change individual elements of it. | | ||
| `//+listType=set` | Applicable to objects that are of type list, and the elements are unique and scalar. Different actors can update individual elements in the list. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the restriction for set is that elements have to be unique and atomic (scalar or atomic list/struct/map).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I agree with that restriction. But, I think currently we only correctly handle the case of scalar set elements, and have a TODO in the structured-merge-diff repo to support any atomic type as an element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jennybuckley and I were talking about lists semantic. I was curious what a "random list with various items" should be? It sounds like it should be "atomic". We should make it clear that UNLESS you know that your list is a set or a map, then it should be atomic. I'm curious if we should make atomic the default. |
/lgtm Jordan, anything to add? Thanks. |
Where it gets confusing (in my head at least) is that set/map seem to refer to the elements of the list, and describe whether they're scalar or nested, and atomic/granular seem to refer to the replacement behaviour supported by the list (and describe if the list must be replaced as a whole or can be replaced element by element). In theory the two could be orthogonal, i.e. it could be valid to have an atomic or granular set, atomic or granular map. Whether that would make sense in practice is a different conversation :) |
I don't know if that's true. |
Do we still need to document the patch stuff in both comments and the field's tag? For example, I currently see this in https://github.com/kubernetes/api/blob/master/apps/v1/types.go:
|
I don't think so. I'm not even sure why we still have these? @mariantalla that's also something we could be doing, look at the code to understand if this is still required and provide guidance for the future. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle stale Just checking in on this -- at this point is the only remaining item the comment from @MikeSpreitzer and @apelisse brought up? |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle stale |
This is now approaching a year old. =/ |
We have some of that in the docs, not in the api convention, which may or may not be good enough. |
@mengqiy Do you know if the apiserver still uses the struct tags or the openapi to merge SMP? I don't remember how it's choosing one or another. |
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
@fejta-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Should eventually address #3752.
Also related: kubernetes-sigs/structured-merge-diff#115
This PR adds a section in
contributors/devel/sig-architecture/api-conventions.md
that references available go markers that can be used to define merge strategies during PATCH operations.Would love feedback on:
Remaining tasks in this PR:
listType
,mapType
,structType
./cc @apelisse
/kind documentation
/sig api-machinery
/sig architecture
(^^as owners of the doc)